fix(http): add content-type override support for http request by joshgerdes · Pull Request #10211 · angular/angular

joshgerdes · 2016-07-21T14:26:35Z

Please check if the PR fulfills these requirements

The commit message follows our guidelines: https://github.com/angular/angular/blob/master/CONTRIBUTING.md#commit-message-format
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

What kind of change does this PR introduce? (check one with "x")

[x] Bugfix
[ ] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Other... Please describe:

What is the current behavior? (You can also link to an open issue here)
see #10210

What is the new behavior?
see #10210

Does this PR introduce a breaking change? (check one with "x")

[ ] Yes
[x] No

If this PR contains a breaking change, please describe the impact and migration path for existing applications: ...

Other information:

Fix static request to allow the ‘Content-Type’ of a http request to be set from the headers options of the call.

vicb · 2016-07-21T14:51:26Z

Is this right ?

No, that is a ridiculous typo. Sorry, will fix.

vicb · 2016-07-21T14:56:38Z

Run gulp as indicated in the contributing docs to fix the CI
Address inline comments
What about header case ? Can't remember of they are case sensitive
Thanks

joshgerdes · 2016-07-21T15:11:31Z

Header field names are case-insensitive:
https://www.w3.org/Protocols/rfc2616/rfc2616-sec4.html#sec4.2

I will change to lowercase.

And is the API update the gulp task I missed?
gulp public-api:update

vicb · 2016-07-21T16:00:18Z

gulp public-api:update

Yep, it will update the .d.ts golden files with public API changes. Read my latest inline comments and figure out if the public API needs to be updated ?

googlebot · 2016-07-21T16:45:32Z

We found a Contributor License Agreement for you (the sender of this pull request) and all commit authors, but as best as we can tell these commits were authored by someone else. If that's the case, please add them to this pull request and have them confirm that they're okay with these commits being contributed to Google. If we're mistaken and you did author these commits, just reply here to confirm.

googlebot · 2016-07-21T16:55:46Z

CLAs look good, thanks!

vicb · 2016-07-21T17:50:57Z

Please merge the commits and add a test

arciisine · 2016-07-21T19:55:52Z

If a user has specified a content-type that isn't in a case statement, won't default override that? Should detecting from the body only occur if the content type is not specified?

Yes, if not found it would default to whatever type it determined from the body. I was assuming that the limited content types specified as enums were the 'supported' types. And in the future the switch statement could be expanded as more enums are added/supported.

IIUC this PR does not change the behavior vs the previous impl.

Sometimes I still want to send an empty content-type header. Now it always detects it from the body, can I somehow override that?

joshgerdes · 2016-07-21T20:29:18Z

Tests added and squashed the commits.

vicb · 2016-07-21T20:48:15Z

Please add a return type

Fix static request to allow the ‘Content-Type’ of a http request to be set from the headers options of the call.

joshgerdes · 2016-07-22T01:07:34Z

Updated.

And yes, that missing semi-colon really did mess with the formatter.

…ar#10211)

angular-automatic-lock-bot · 2019-09-12T17:52:14Z

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}

googlebot added the cla: yes label Jul 21, 2016

vicb reviewed Jul 21, 2016
View reviewed changes

vicb added the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Jul 21, 2016

googlebot added cla: no and removed cla: yes labels Jul 21, 2016

joshgerdes force-pushed the fix-http-contenttype branch from 52dcd45 to c665bd5 Compare July 21, 2016 16:55

googlebot added cla: yes and removed cla: no labels Jul 21, 2016

arciisine reviewed Jul 21, 2016
View reviewed changes

joshgerdes force-pushed the fix-http-contenttype branch from 38923d9 to fd6448c Compare July 21, 2016 20:27

vicb reviewed Jul 21, 2016
View reviewed changes

Comment thread modules/@angular/http/src/static_request.ts Outdated

Copy link
Copy Markdown

Contributor

vicb Jul 21, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a return type

fix(http): add content-type override support for http request

cb937bc

Fix static request to allow the ‘Content-Type’ of a http request to be set from the headers options of the call.

joshgerdes force-pushed the fix-http-contenttype branch from fd6448c to cb937bc Compare July 21, 2016 21:30

vicb merged commit bdb5912 into angular:master Jul 22, 2016

hbkrunal pushed a commit to hbkrunal/angular that referenced this pull request Sep 15, 2016

feat(http): add content-type override support for http request (angul…

a1fa9f9

…ar#10211)

angular-automatic-lock-bot Bot locked and limited conversation to collaborators Sep 12, 2019

Conversation

joshgerdes commented Jul 21, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vicb Jul 21, 2016

Choose a reason for hiding this comment

Uh oh!

joshgerdes Jul 21, 2016

Choose a reason for hiding this comment

Uh oh!

vicb commented Jul 21, 2016

Uh oh!

joshgerdes commented Jul 21, 2016

Uh oh!

vicb commented Jul 21, 2016

Uh oh!

googlebot commented Jul 21, 2016

Uh oh!

googlebot commented Jul 21, 2016

Uh oh!

vicb commented Jul 21, 2016

Uh oh!

arciisine Jul 21, 2016

Choose a reason for hiding this comment

Uh oh!

joshgerdes Jul 21, 2016

Choose a reason for hiding this comment

Uh oh!

vicb Jul 21, 2016

Choose a reason for hiding this comment

Uh oh!

slavede Oct 4, 2017

Choose a reason for hiding this comment

Uh oh!

joshgerdes commented Jul 21, 2016

Uh oh!

vicb Jul 21, 2016

Choose a reason for hiding this comment

Uh oh!

joshgerdes commented Jul 22, 2016

Uh oh!

angular-automatic-lock-bot Bot commented Sep 12, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

joshgerdes commented Jul 21, 2016 •

edited

Loading